Skip to content

fix(query-core): align focusManager with batching and error handling #9840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

fix(query-core): align focusManager with batching and error handling #9840

wants to merge 2 commits into from
+6 −3

Conversation

@LeDevNovice
Copy link

@LeDevNovice LeDevNovice commented Nov 2, 2025 •
edited by coderabbitai bot
Loading

🎯 Changes

This PR addresses a few inconsistencies I noticed in focusManager.ts while reviewing the code.

Changes:
- Adds notifyManager.batch() to onFocus() : This aligns the focusManager with other managers and prevents potential cascading re-renders in React from a single focus event.
- Adds : void return type to onUnsubscribe() : A minor type consistency fix to match onSubscribe().

I'm happy to make any adjustments based on your feedback. Thanks !

✅ Checklist

  • I have followed the steps in the Contributing guide.
  • I have tested this code locally with pnpm run test:pr.

🚀 Release Impact

  • This change affects published code, and I have generated a changeset.
  • This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

  • Bug Fixes
    • Improved error handling for focus listener notifications so a failing listener no longer prevents others from running.
    • Enhanced notification batching to improve performance and reliability when dispatching focus-related updates.

@changeset-bot
Copy link

changeset-bot bot commented Nov 2, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: bc84d95

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 2, 2025 •
edited
Loading

Walkthrough

The focus manager's onFocus now invokes listeners inside notifyManager.batch and wraps each listener call in a try/catch that logs errors and continues; onUnsubscribe's signature was changed to explicitly return void.

Changes

Cohort / File(s) Summary
Focus manager enhancement
packages/query-core/src/focusManager.ts		 Added notifyManager import; wrapped onFocus listener notifications in notifyManager.batch with per-listener try/catch and error logging; changed protected onUnsubscribe() signature to protected onUnsubscribe(): void

Sequence Diagram(s)

sequenceDiagram
    participant FocusMgr as FocusManager
    participant Notify as notifyManager
    participant Listener as Listener[N]

    FocusMgr->>Notify: batch(fn)
    Note right of Notify #DDFFDD: batched execution
    Notify->>FocusMgr: execute fn
    FocusMgr->>Listener: for each listener -> try { listener() } catch (e) { log(e) }
    Note right of FocusMgr #FFFBCC: per-listener try/catch ensures continuation
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Review notifyManager.batch usage for correct scoping and async behavior.
  • Confirm error logging detail and that swallowing errors is intentional.
  • Verify onUnsubscribe(): void matches interface/overrides and build/type checks.

Poem

🐰 I hop, I batch, I call each friend,

A gentle try, if errors send.
No single fall will stop the race —
Focus blooms in steady pace. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix(query-core): align focusManager with batching and error handling" is clearly related to the main changes in the PR. It directly references the two primary modifications: adding batching support and implementing error handling in the focusManager. The title is concise, specific, and accurately summarizes the key improvements without being vague or overly broad. A developer scanning the commit history would immediately understand that this PR addresses alignment and resilience issues in the focusManager.
Description Check ✅ Passed The PR description follows the repository template structure with all required sections present. The 🎯 Changes section clearly describes the modifications (batch wrapping, error handling per listener, and return type consistency) along with their motivations (preventing cascading re-renders, isolating errors, and type alignment). The ✅ Checklist section is fully completed with both items checked. The 🚀 Release Impact section is present but neither option is selected, which creates some ambiguity about whether a changeset should have been generated for these published code changes, though this represents a minor incompleteness rather than a missing section.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1c72b84 and bc84d95.

📒 Files selected for processing (1)
  • packages/query-core/src/focusManager.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/query-core/src/focusManager.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Nov 2, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40b296b and 1c72b84.

📒 Files selected for processing (1)
  • packages/query-core/src/focusManager.ts (3 hunks)
🔇 Additional comments (2)
packages/query-core/src/focusManager.ts (2)

3-3: LGTM!

The import of notifyManager is necessary for the batching functionality added to onFocus().

42-42: LGTM!

Adding the explicit void return type improves consistency with onSubscribe() on line 36.

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 5, 2025

I don’t think batching is necessary here as the subscribers aren’t react components. We also don’t batch in the onlineManager.

@TkDodo TkDodo closed this Nov 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

package: query-core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@LeDevNovice @TkDodo